Implement TODOs and add tests #15535
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
Generated by 🚫 Danger |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great improvement, replacing fatalError calls with proper error handling through a new DecodingError enum. This makes the Generable decoding much more robust. The addition of a comprehensive test suite is also a valuable contribution, covering various success and failure scenarios for decoding. My feedback focuses on a few minor points: removing a couple of leftover TODO comments, simplifying some code for better clarity, and removing a redundant test case.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant improvement, addressing TODOs by replacing fatalError calls with proper, typed error handling. The introduction of ModelOutput.DecodingError and its usage throughout the Generable initializers makes the decoding process more robust. The addition of a comprehensive test suite in GenerableTests.swift is excellent, covering success cases, various error conditions, and schema validation. My feedback includes a few suggestions to further refine error reporting for numeric conversions to be more descriptive, and a minor cleanup to remove a redundant check.
paulb777
force-pushed
the
pb-tests-todos
branch
4 times, most recently
from
59c70a6 to
bcd9e06
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an excellent pull request that significantly improves the robustness and test coverage of the codebase. Replacing fatalError with proper error handling is a crucial change, and the new DecodingError enum is well-designed. The addition of comprehensive unit tests for various decoding scenarios, including nested objects and error conditions, is a great contribution. The code is now cleaner and more reliable. I have one minor suggestion to make a test assertion more robust.
| _ = try Person(modelOutput) | ||
| Issue.record("Did not throw an error.") | ||
| } catch let GenerativeModel.GenerationError.decodingFailure(context) { | ||
| #expect(context.debugDescription.contains("\"forty\" does not contain Int")) |
There was a problem hiding this comment.
The assertion on the debugDescription of the error seems to be checking for a substring that won't be present. The decodingFailure helper creates a message like FirebaseAILogic.ModelOutput does not contain Int.\nContent: "forty". The current check for \"forty\" does not contain Int will likely fail. It's better to check for the relevant parts of the error message separately to make the test more robust.
#expect(context.debugDescription.contains("does not contain Int") && context.debugDescription.contains("\"forty\""))
paulb777
force-pushed
the
pb-tests-todos
branch
from
bcd9e06 to
0db7eda
Compare
3 participants
@andrewheard Feel free to merge if close or useful.